[Feature] Datacatalog cache deletion #4655

pvditt · 2023-12-30T02:23:17Z

Tracking issue

Why are the changes needed?

Flyte doesn't support the deletion of cached task executions. Adding cache eviction/cleanup gives Flyte users more control over their data helping with things such as GDPR compliance + reducing their cache size.

Opted to support just single task executions as invalidating an entire workflow's cache can run into timeout issues going down the nesting of nodes.

What changes were proposed in this pull request?

add flyteadmin endpoint to support the deletion of single task executions
add catalog endpoint to support the deletion of single task executions

How was this patch tested?

unit tests
Run workflow that caches task executions

grpcurl -plaintext -d '{"task_execution_id": {"node_execution_id": {"execution_id": {"project": "flytesnacks", "domain": "development", "name": "fbcf25e4303104b4b81e"}, "node_id": "n0"}, "task_id": {"project": "flytesnacks", "domain": "development", "name": "basics.cache_test.basic_cache", "version": "98XuDRSQW4hVWKYQdQfd1Q=="}, "retry_attempt": 0}}' localhost:8089 flyteidl.service.CacheService.EvictTaskExecutionCache

Setup process

if want to verify from console, need to pull down: feat: add cache evict status flyteconsole#835 and then manually update flyteconsole/packages/flyteidl-types/node_modules/@flyteorg/flyteidl/gen/pb-js/flyteidl.d.ts

        enum CatalogCacheStatus {
            CACHE_DISABLED = 0,
            CACHE_MISS = 1,
            CACHE_HIT = 2,
            CACHE_POPULATED = 3,
            CACHE_LOOKUP_FAILURE = 4,
            CACHE_PUT_FAILURE = 5,
            CACHE_SKIPPED = 6,
            CACHE_EVICTED = 7
        }

(NOTE - a follow up PR will be to get that change merged into console since console doesn't support CACHE_EVICT CatalogCacheStatus)

can also verify by checking artifact_data and artifacts tables

Screenshots

Check all the applicable boxes

I updated the documentation accordingly.
All new and existing tests passed.
All commits are signed-off.

Related PRs

Docs link

Implemented new datacatalog functionality required for cache eviction Updated to latest unreleased version of flyteidl and flyteplugins Signed-off-by: Nick Müller <[email protected]>

Allows for fields to be explicity set/updated to nil Signed-off-by: Nick Müller <[email protected]>

Allows for re-use by cache manager Signed-off-by: Nick Müller <[email protected]>

Added endpoint for evicting execution cache Added endpoint for evicting task execution cache Signed-off-by: Nick Müller <[email protected]>

Signed-off-by: Nick Müller <[email protected]>

Extended reservation retrieval to allow querying via artifact tag in catalog client interface Signed-off-by: Nick Müller <[email protected]>

Added method to delete catalog artifact by ID Signed-off-by: Nick Müller <[email protected]>

Signed-off-by: Nick Müller <[email protected]>

…epropeller and flytestdlib Signed-off-by: Nick Müller <[email protected]>

Signed-off-by: Nick Müller <[email protected]>

Added new CacheEvictionError message representing an error encountered during eviction of stored data Added new UpdateTaskExecution endpoint for updating task executions, currently only supporting cache eviction Signed-off-by: Nick Müller <[email protected]>

Ran go mod tidy Signed-off-by: Nick Müller <[email protected]>

Signed-off-by: Nick Müller <[email protected]>

…oints Signed-off-by: Nick Müller <[email protected]>

Signed-off-by: Nick Müller <[email protected]>

grpc-gateway parsing of URL params does not work for joined endpoint at the moment - fixed in major version upgrade Added extra CacheEvictionErrorCode enum entries Signed-off-by: Nick Müller <[email protected]>

…artifacts to datacatalog Signed-off-by: Nick Müller <[email protected]>

Signed-off-by: Nick Müller <[email protected]>

Implement deleting of artifacts as bulk operation Signed-off-by: Nick Müller <[email protected]>

Signed-off-by: Paul Dittamo <[email protected]>

codecov · 2023-12-30T02:28:31Z

Codecov Report

Attention: Patch coverage is 67.56098% with 133 lines in your changes are missing coverage. Please review.

Project coverage is 59.07%. Comparing base (e07084c) to head (9e322a8).

Files	Patch %	Lines
flyteadmin/pkg/manager/impl/cache_manager.go	68.18%	65 Missing and 5 partials ⚠️
datacatalog/pkg/repositories/gormimpl/artifact.go	75.92%	9 Missing and 4 partials ⚠️
flytestdlib/catalog/client.go	0.00%	11 Missing ⚠️
flytestdlib/catalog/datacatalog/transformer.go	0.00%	10 Missing ⚠️
flytestdlib/catalog/noop_catalog.go	0.00%	10 Missing ⚠️
...atacatalog/pkg/manager/impl/reservation_manager.go	0.00%	5 Missing and 2 partials ⚠️
datacatalog/pkg/manager/impl/artifact_manager.go	88.00%	5 Missing and 1 partial ⚠️
flytestdlib/catalog/datacatalog/datacatalog.go	87.23%	4 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4655      +/-   ##
==========================================
+ Coverage   59.00%   59.07%   +0.07%     
==========================================
  Files         645      647       +2     
  Lines       55578    55972     +394     
==========================================
+ Hits        32792    33065     +273     
- Misses      20194    20301     +107     
- Partials     2592     2606      +14

Flag	Coverage Δ
unittests	`59.07% <67.56%> (+0.07%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…on-past-executions' into feature/datacatalog-cache-deletion

Signed-off-by: Paul Dittamo <[email protected]>

…che evicted Signed-off-by: Paul Dittamo <[email protected]>

Signed-off-by: Paul Dittamo <[email protected]>

datacatalog/pkg/manager/impl/artifact_manager.go

Signed-off-by: Paul Dittamo <[email protected]>

…ations Signed-off-by: Paul Dittamo <[email protected]>

Signed-off-by: Paul Dittamo <[email protected]>

hamersaw

lets cleanup imports and resolve merge conflicts and then merge!

flyteidl/protos/flyteidl/service/cache.proto

flytepropeller/pkg/controller/nodes/array/handler_test.go

flytepropeller/pkg/controller/nodes/executor_test.go

flytepropeller/pkg/controller/workflow/executor_test.go

Signed-off-by: Paul Dittamo <[email protected]>

pvditt · 2024-03-12T21:42:27Z

put up change for console: flyteorg/flyteconsole#851

Nick Müller and others added 26 commits December 15, 2022 19:17

Migrated CatalogClient from propeller to stdlib

d5fe607

Implemented new datacatalog functionality required for cache eviction Updated to latest unreleased version of flyteidl and flyteplugins Signed-off-by: Nick Müller <[email protected]>

Added function for updating selected fields of NodeExecutionModel

422eb46

Allows for fields to be explicity set/updated to nil Signed-off-by: Nick Müller <[email protected]>

Refactored listing of node and task executions to shared util

47e9a00

Allows for re-use by cache manager Signed-off-by: Nick Müller <[email protected]>

Implemented CacheService

25c187c

Added endpoint for evicting execution cache Added endpoint for evicting task execution cache Signed-off-by: Nick Müller <[email protected]>

Updated to latest unreleased versions of flytepropeller and flytestdlib

dc59334

Signed-off-by: Nick Müller <[email protected]>

Added function for deleting artifacts to catalog client interface

4c041c1

Extended reservation retrieval to allow querying via artifact tag in catalog client interface Signed-off-by: Nick Müller <[email protected]>

Added method to release catalog reservation by artifact tag

56c7b31

Added method to delete catalog artifact by ID Signed-off-by: Nick Müller <[email protected]>

Update to latest unreleased versions of flyteidl and flyteplugins

557a991

Signed-off-by: Nick Müller <[email protected]>

Updated to latest unreleased versions of flyteidl, flyteplugins, flyt…

821467e

…epropeller and flytestdlib Signed-off-by: Nick Müller <[email protected]>

Added datacatalog endpoint for deleting artifacts

4dd0afd

Signed-off-by: Nick Müller <[email protected]>

Updated to latest released version of flytestdlib

6480f02

Ran go mod tidy Signed-off-by: Nick Müller <[email protected]>

Refactored cache eviction to own service

1dc2115

Signed-off-by: Nick Müller <[email protected]>

Refactored new cache service to share EvictCacheRequest for both endp…

419570e

…oints Signed-off-by: Nick Müller <[email protected]>

Merged cache eviction endpoints into single RPC call

ea6df4e

Signed-off-by: Nick Müller <[email protected]>

Removed no longer relevant generated mocks

da32ef0

Signed-off-by: Nick Müller <[email protected]>

Split CacheService.EvictCache into two separate endpoints

80e8e72

grpc-gateway parsing of URL params does not work for joined endpoint at the moment - fixed in major version upgrade Added extra CacheEvictionErrorCode enum entries Signed-off-by: Nick Müller <[email protected]>

Add bulk endpoints for acquiring/releasing reservations and deleting …

d7f5203

…artifacts to datacatalog Signed-off-by: Nick Müller <[email protected]>

Implemented deletion of artifact including artifact data

d7eda41

Signed-off-by: Nick Müller <[email protected]>

Deleting artifacts cleans up tags and partitions as well

8890348

Signed-off-by: Nick Müller <[email protected]>

Updated to latest unreleased version of flyteidl and flytestdlib

a2315f8

Signed-off-by: Nick Müller <[email protected]>

Updated to latest unreleased version of flytestdlib

d2565a1

Signed-off-by: Nick Müller <[email protected]>

Updated to latest unreleased version fo flyteidl and flytestdlib

0167db1

Signed-off-by: Nick Müller <[email protected]>

Implement acquiring/releasing of reserverations as bulk operation

8f2f414

Implement deleting of artifacts as bulk operation Signed-off-by: Nick Müller <[email protected]>

Transfer commits

c2d5454

Signed-off-by: Paul Dittamo <[email protected]>

resolve conflicts

5cd55cf

Signed-off-by: Paul Dittamo <[email protected]>

pvditt added 3 commits January 3, 2024 13:42

Merge remote-tracking branch 'blackshark-ai/flyteplugins/cache-evicti…

d79a634

…on-past-executions' into feature/datacatalog-cache-deletion

resolve conflicts

adf6209

Signed-off-by: Paul Dittamo <[email protected]>

update import paths

23d2dc1

Signed-off-by: Paul Dittamo <[email protected]>

pvditt mentioned this pull request Jan 9, 2024

add cache evicted status #4705

Merged

3 tasks

pvditt added 7 commits January 10, 2024 09:15

Merge branch 'master' into feature/datacatalog-cache-deletion

d01a0e7

Signed-off-by: Paul Dittamo <[email protected]>

set node execution CacheStatus and taskNodeMetadata CacheStatus to ca…

d7ac714

…che evicted Signed-off-by: Paul Dittamo <[email protected]>

Merge branch 'master' into feature/datacatalog-cache-deletion

409e221

Signed-off-by: Paul Dittamo <[email protected]>

remove bulk endpoints

6aeaae1

Signed-off-by: Paul Dittamo <[email protected]>

remove bulk delete counters

706dc4e

Signed-off-by: Paul Dittamo <[email protected]>

revert mod tidy chagnes

6ec0e88

Signed-off-by: Paul Dittamo <[email protected]>

replace flyteidl source in flytestdlib

3ce3742

Signed-off-by: Paul Dittamo <[email protected]>

pvditt commented Jan 31, 2024

View reviewed changes

datacatalog/pkg/manager/impl/artifact_manager.go Show resolved Hide resolved

pvditt added 4 commits January 31, 2024 11:02

add otelgrpc dependency

209e2aa

Signed-off-by: Paul Dittamo <[email protected]>

fix flytestdlib dependencies

007be11

Signed-off-by: Paul Dittamo <[email protected]>

revert release reservation changes to support release multiple reserv…

13a2f68

…ations Signed-off-by: Paul Dittamo <[email protected]>

tidy

2f9c0b4

Signed-off-by: Paul Dittamo <[email protected]>

pvditt marked this pull request as ready for review January 31, 2024 20:07

dosubot bot added size:XXL This PR changes 1000+ lines, ignoring generated files. enhancement New feature or request labels Jan 31, 2024

pvditt added 4 commits January 31, 2024 12:51

revert tidy

91d30e4

Signed-off-by: Paul Dittamo <[email protected]>

tidy 🤦

30eb267

Signed-off-by: Paul Dittamo <[email protected]>

Merge branch 'master' into feature/datacatalog-cache-deletion

a0d0aae

Signed-off-by: Paul Dittamo <[email protected]>

tidy

7e8a19e

Signed-off-by: Paul Dittamo <[email protected]>

hamersaw reviewed Feb 13, 2024

View reviewed changes

pvditt added 4 commits February 22, 2024 15:13

merge master

cdbb950

Signed-off-by: Paul Dittamo <[email protected]>

cleanup

6e80529

Signed-off-by: Paul Dittamo <[email protected]>

cleanup

d858906

Signed-off-by: Paul Dittamo <[email protected]>

update boiler plate linter

ae652a0

Signed-off-by: Paul Dittamo <[email protected]>

pvditt requested a review from hamersaw February 23, 2024 04:43

pvditt added 2 commits February 27, 2024 11:04

Merge branch 'master' into feature/datacatalog-cache-deletion

83263a8

Signed-off-by: Paul Dittamo <[email protected]>

merge master

9e322a8

Signed-off-by: Paul Dittamo <[email protected]>

pvditt mentioned this pull request Oct 31, 2024

[Core feature] Add an option to consider deleted cache as cache miss. #5918

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature] Datacatalog cache deletion #4655

[Feature] Datacatalog cache deletion #4655

pvditt commented Dec 30, 2023 •

edited

Loading

codecov bot commented Dec 30, 2023 •

edited

Loading

hamersaw left a comment

pvditt commented Mar 12, 2024

[Feature] Datacatalog cache deletion #4655

Are you sure you want to change the base?

[Feature] Datacatalog cache deletion #4655

Conversation

pvditt commented Dec 30, 2023 • edited Loading

Tracking issue

Why are the changes needed?

What changes were proposed in this pull request?

How was this patch tested?

Setup process

Screenshots

Check all the applicable boxes

Related PRs

Docs link

codecov bot commented Dec 30, 2023 • edited Loading

Codecov Report

hamersaw left a comment

Choose a reason for hiding this comment

pvditt commented Mar 12, 2024

pvditt commented Dec 30, 2023 •

edited

Loading

codecov bot commented Dec 30, 2023 •

edited

Loading